fix: improve project favicon detection for monorepos#1024
fix: improve project favicon detection for monorepos#1024ponbac wants to merge 2 commits intopingdotgg:mainfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
30cac2c to
b936f17
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4a8d330eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
c396c88 to
551d9f2
Compare
|
My 2c
I would add unit tests for this file rather than relying on integration tests. Since this is a new file, I would also add some solid docblocks to provide good human and AI context for the decisions being made. Hopefully the reviewers will pick that up and think it's a good idea for their code standards. |
Not sure about separating them right now, but I also feel like I agree with the unit tests, added now. Also added an explanatory comment to the |
a079d93 to
470ab3d
Compare
9f02bfb to
8cc9e19
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
47c7166 to
b41ebd3
Compare
b3c9960 to
185272a
Compare
185272a to
ea10d7f
Compare

The
XXLtag seems a little weird for this PR? It is a net +288 LoC, excluding test files.What Changed
Improve project favicon detection without introducing a broad recursive search.
Closes #1020.
Rules, in order:
Example:
/repo/favicon.svgor/repo/public/favicon.ico.<link rel="icon" ... href="...">tags or object-style{ href, rel: "icon" }declarations in a small set of common source files, then resolve that target.Example:
/repo/index.htmlpoints to/brand/logo.svg, so the route first tries/repo/public/brand/logo.svgand then/repo/brand/logo.svg.apps/andpackages/, plus other top-level directories in the requested root, using the same non-git directory ignore rules asworkspaceEntries.Example:
/repo/apps/web/public/favicon.pngor/repo/frontend/public/favicon.pngis found when the cwd is/repo.Example:
/repo/favicon.svgwins over/repo/apps/web/public/favicon.ico.I also extracted the shared gitignore probing into
apps/server/src/gitIgnore.tsso bothworkspaceEntriesandprojectFaviconRouteuse the same chunkedgit check-ignore --no-index -z --stdinfiltering. I also extracted the shared workspace directory ignore policy intoapps/server/src/workspaceIgnore.ts, soprojectFaviconRoutenow uses the same non-git skip rules asworkspaceEntrieswhen scanning nested roots instead of keeping a separate ignore list.Why
The current behavior misses common monorepo layouts where the favicon lives under an app directory instead of the repo root.
There was already an earlier PR for this in the upstream repo: #690. That version was larger and messier. This keeps the rules simple on purpose, leaving more extensive changes to project icons for trusted maintainers.
UI Changes
Not applicable.
Checklist
Note
Improve project favicon detection to support monorepos with git-ignore filtering
tryHandleProjectFaviconRequestin projectFaviconRoute.ts as an Effect-based handler that searches the project root, then conventional monorepo subdirectories (apps/,packages/), then other top-level directories, before falling back to an SVG placeholder.filterGitIgnoredPaths(chunkedgit check-ignoreinvocations) andisInsideGitWorkTree, used to skip git-ignored favicon candidates and source files.gitIgnore.FileSystem, provided viaprovideServicein wsServer.ts.Macroscope summarized ea10d7f.
Note
Medium Risk
Moderate risk: rewrites the favicon HTTP route to new Effect-based filesystem and git-aware search logic, which could change responses/perf and has more path-handling edge cases despite added containment checks and tests.
Overview
Improves
/api/project-faviconresolution to support monorepo layouts by searching the requested root first, then scanning likely child app/package directories and parsing a small set of source files forrel=iconmetadata before falling back to the generated SVG.Adds shared
gitIgnoreutilities (isInsideGitWorkTree, chunkedfilterGitIgnoredPaths,splitNullSeparatedPaths) and applies gitignore/workspace-directory ignore filtering to favicon candidate paths, source probes, and nested search roots (including caching of gitignore decisions).Refactors
workspaceEntriesto consume the new sharedgitIgnore+workspaceIgnoremodules, and updateswsServer/tests to call the favicon handler as anEffect(with FileSystem provided) while expanding route test coverage for ordering, unreadable files, and gitignored paths.Written by Cursor Bugbot for commit ea10d7f. This will update automatically on new commits. Configure here.